chore(preconf): enrich sequencer server health check impl.#3086
chore(preconf): enrich sequencer server health check impl.#3086bar-bera wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enriches the sequencer preconf server /eth/v1/preconf/health endpoint by reporting readiness/sync state and execution-layer connectivity, wiring in existing node services to drive those checks.
Changes:
- Extend
preconf.Serverto acceptSyncCheckerandELCheckerdependencies and use them in the health handler to return 200 vs 503 plus a JSON body. - Wire
ConsensusServiceandEngineClientinto the sequencer preconf server provider. - Add
HealthResponsetype and new unit tests covering the health endpoint status/flags.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| node-core/components/preconf_server.go | Injects consensus + engine clients into the preconf server so health checks can reflect node/EL state. |
| beacon/preconf/types.go | Adds HealthResponse struct returned by /eth/v1/preconf/health. |
| beacon/preconf/server.go | Implements health response building and 200/503 logic based on readiness, syncing, and EL connectivity. |
| beacon/preconf/server_test.go | Updates server construction for new deps and adds coverage for the health endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## preconf-dev #3086 +/- ##
===============================================
+ Coverage 58.82% 59.22% +0.40%
===============================================
Files 381 381
Lines 19687 19714 +27
===============================================
+ Hits 11581 11676 +95
+ Misses 7138 7067 -71
- Partials 968 971 +3
🚀 New features to boost your workflow:
|
calbera
left a comment
calbera
left a comment
There was a problem hiding this comment.
one nit but mostly looks good
| resp := new(HealthResponse) | ||
|
|
||
| if s.syncChecker != nil { | ||
| resp.IsReady = s.syncChecker.IsAppReady() == nil |
There was a problem hiding this comment.
IsAppReady should return a boolean (in general any func thats named Is<something> should return bool) since the error value is never used.
There was a problem hiding this comment.
agree it's a bit weird but this is just the ConsensusService relaying the comet interface. I would avoid to wrap it with an extra layer
| // IsAppReady returns nil if the node has committed at least one block. | ||
| IsAppReady() error |
There was a problem hiding this comment.
nit: to make it clear from reading this code, we can add a quick comment here explaining:
- why/how we use the error
- why its not overriden (bc its part of the comet service already)
Uses existent services to add checks on the
/eth/v1/preconf/healthendpoint of the sequencer server: